feat: Introduce tiered timeout system with per-endpoint configuration#653
feat: Introduce tiered timeout system with per-endpoint configuration#653
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #653 +/- ##
==========================================
+ Coverage 96.57% 96.74% +0.17%
==========================================
Files 45 45
Lines 4318 4336 +18
==========================================
+ Hits 4170 4195 +25
+ Misses 148 141 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9653d95 to
634577c
Compare
- Add `Timeout` type alias (`timedelta | Literal['no_timeout'] | None`) to `_consts.py` and export it from the public API - Accept `None` on abstract `HttpClient.call()` / `HttpClientAsync.call()` so the HTTP client resolves its own default timeout internally - Move `_calculate_timeout` into the impit HTTP client implementation since exponential timeout growth on retries is client-specific logic - Add `timeout` parameter to all public resource client methods for user-controllable per-request HTTP timeouts - Add timeout parameter to base methods `_list()`, `_create()`, and `_get_or_create()` which previously lacked it - Rename domain-specific Actor/Task run timeout from `timeout` to `run_timeout` to avoid ambiguity with the HTTP request timeout - Update docstrings to document three-state timeout semantics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce `timeout_max` parameter to cap exponential timeout growth independently from the initial timeout. Lower defaults to `DEFAULT_REQUEST_TIMEOUT=10s`, `DEFAULT_REQUEST_TIMEOUT_MAX=600s`, and `DEFAULT_MAX_RETRIES=4`, producing the sequence [10, 20, 40, 80, 160]. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
634577c to
ac20367
Compare
Pijukatel
left a comment
There was a problem hiding this comment.
I would be very careful about changing the actual timeouts. Can we please split this into 2 PRs:
- Refactoring (majority)
- Assigning different timeouts to API operations
5071549 to
3d9f230
Compare
3d9f230 to
0ad2c46
Compare
There was a problem hiding this comment.
Pull request overview
Introduces a tiered timeout system across the Apify Python client, enabling per-endpoint timeout configuration via a shared Timeout type and centralized timeout resolution in HTTP clients.
Changes:
- Added timeout tiers (
short/medium/long+no_timeout) and propagated atimeoutparameter through most resource-client methods. - Updated HTTP client internals to resolve timeout tiers and apply exponential per-attempt timeout growth capped by
timeout_max. - Updated tests/docs for the new timeout API and reduced default retries (
DEFAULT_MAX_RETRIES8 → 4).
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_pluggable_http_client.py | Updates fake custom HTTP clients and tests to use Timeout and pass per-call timeout tiers. |
| tests/unit/test_http_clients.py | Adjusts base HTTP client init test to the new tiered timeout fields. |
| tests/unit/test_client_timeouts.py | Adds unit coverage for tier resolution, no_timeout, and timeout growth/capping. |
| tests/unit/test_actor_start_params.py | Updates Actor start tests to use run_timeout instead of timeout. |
| tests/integration/test_webhook.py | Fixes integration test type expectation for Actor call result. |
| src/apify_client/_types.py | Introduces Timeout and consolidates JsonSerializable plus minimal polling models. |
| src/apify_client/_resource_clients/webhook_dispatch_collection.py | Adds timeout parameter to list operations and forwards to _list. |
| src/apify_client/_resource_clients/webhook_dispatch.py | Adds timeout parameter to get operations and forwards to _get. |
| src/apify_client/_resource_clients/webhook_collection.py | Adds timeout parameter to list/create and forwards to _list/_create. |
| src/apify_client/_resource_clients/webhook.py | Adds timeout parameter to CRUD/test methods and forwards to HTTP calls. |
| src/apify_client/_resource_clients/user.py | Adds timeout parameter to user endpoints and forwards to _get/_http_client.call. |
| src/apify_client/_resource_clients/task_collection.py | Adds request timeout and renames run timeout → run_timeout for create. |
| src/apify_client/_resource_clients/task.py | Adds request timeout broadly; renames run timeout → run_timeout for start/call/update. |
| src/apify_client/_resource_clients/store_collection.py | Adds timeout parameter to list and forwards to _list. |
| src/apify_client/_resource_clients/schedule_collection.py | Adds timeout parameter to list/create and forwards to _list/_create. |
| src/apify_client/_resource_clients/schedule.py | Adds timeout parameter to schedule operations and forwards to _get/_update/_delete. |
| src/apify_client/_resource_clients/run_collection.py | Adds timeout parameter to run listing and forwards to _list. |
| src/apify_client/_resource_clients/run.py | Adds request timeout widely; renames run timeout → run_timeout where applicable; defaults waiters to no_timeout. |
| src/apify_client/_resource_clients/request_queue_collection.py | Adds timeout parameter to list/get_or_create and forwards to _list/_get_or_create. |
| src/apify_client/_resource_clients/request_queue.py | Replaces old fixed constants with tiered timeout parameters for queue operations. |
| src/apify_client/_resource_clients/log.py | Adds timeout parameter to log get/stream methods and forwards to HTTP calls. |
| src/apify_client/_resource_clients/key_value_store_collection.py | Adds timeout parameter to list/get_or_create and forwards to _list/_get_or_create. |
| src/apify_client/_resource_clients/key_value_store.py | Replaces old fixed constants with tiered timeout parameters across KVS operations. |
| src/apify_client/_resource_clients/dataset_collection.py | Adds timeout parameter to list/get_or_create and forwards to _list/_get_or_create. |
| src/apify_client/_resource_clients/dataset.py | Replaces old fixed constants with tiered timeout parameters across dataset operations. |
| src/apify_client/_resource_clients/build_collection.py | Adds timeout parameter to list and forwards to _list. |
| src/apify_client/_resource_clients/build.py | Adds timeout parameter to build operations and forwards to _get/_delete/_wait_for_finish. |
| src/apify_client/_resource_clients/actor_version_collection.py | Adds timeout parameter to list/create and forwards to _list/_create. |
| src/apify_client/_resource_clients/actor_version.py | Adds timeout parameter to get/update/delete and forwards to _get/_update/_delete. |
| src/apify_client/_resource_clients/actor_env_var_collection.py | Adds timeout parameter to list/create and forwards to _list/_create. |
| src/apify_client/_resource_clients/actor_env_var.py | Adds timeout parameter to get/update/delete and forwards to _get/_update/_delete. |
| src/apify_client/_resource_clients/actor_collection.py | Adds timeout parameter to list/create and forwards to _list/_create. |
| src/apify_client/_resource_clients/actor.py | Adds request timeout widely; renames run timeout → run_timeout for start/call. |
| src/apify_client/_resource_clients/_resource_client.py | Makes internal CRUD/list/create helpers require timeout and forwards it to HTTP client calls. |
| src/apify_client/_http_clients/_impit.py | Updates Impit clients to use tiered timeouts and per-attempt computed timeouts. |
| src/apify_client/_http_clients/_base.py | Implements _compute_timeout() tier resolution + exponential growth capped by timeout_max; updates call signatures to Timeout. |
| src/apify_client/_consts.py | Replaces single default timeout + old operation constants with tier defaults and DEFAULT_TIMEOUT_MAX; lowers DEFAULT_MAX_RETRIES. |
| src/apify_client/_apify_client.py | Changes client constructors to accept tiered timeouts and passes them into default HTTP clients. |
| src/apify_client/init.py | Exports Timeout from the package top-level. |
| docs/02_concepts/code/05_retries_sync.py | Updates docs example to use new retry/timeout configuration arguments. |
| docs/02_concepts/code/05_retries_async.py | Updates docs example to use new retry/timeout configuration arguments. |
Comments suppressed due to low confidence (1)
src/apify_client/_types.py:20
- The
JsonSerializabledocstring referencesjson.parse, which doesn’t exist in Python’s stdlib (json.loads/json.dumpsare the relevant APIs). Consider correcting the reference and removing the anecdotal claim about approval, so the type alias docs stay accurate and focused.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| token: str | None = None, | ||
| timeout: timedelta = DEFAULT_TIMEOUT, | ||
| timeout_short: timedelta = DEFAULT_TIMEOUT_SHORT, | ||
| timeout_medium: timedelta = DEFAULT_TIMEOUT_MEDIUM, | ||
| timeout_long: timedelta = DEFAULT_TIMEOUT_LONG, | ||
| timeout_max: timedelta = DEFAULT_TIMEOUT_MAX, |
There was a problem hiding this comment.
ImpitHttpClient.__init__ dropped the old timeout= argument in favor of tiered timeouts. Since ImpitHttpClient is part of the public API, this is a breaking change; consider supporting timeout as a deprecated alias (e.g., set timeout_long and/or all tiers) to keep existing user code working until the next major version.
| def test_no_timeout_passes_none_to_impit_sync(patch_request: list) -> None: | ||
| """Test that `'no_timeout'` passes `timeout=None` to impit (uses client-level default).""" | ||
| client = ImpitHttpClient(timeout_short=timedelta(seconds=10)) |
There was a problem hiding this comment.
These tests state that 'no_timeout' passes timeout=None to impit “(uses client-level default)”, but the HTTP client docs describe 'no_timeout' as disabling the timeout entirely. Please reword the docstrings to match the intended semantics (disable timeout / pass-through None) to avoid confusion.
| params=request_params, | ||
| json=list(request_batch), | ||
| timeout=STANDARD_OPERATION_TIMEOUT, | ||
| timeout='medium', |
There was a problem hiding this comment.
batch_add_requests(..., timeout=...) exposes a timeout parameter but the worker call is hard-coded to timeout='medium', so the user-provided timeout is ignored. Thread the timeout argument through to _batch_add_requests_worker (or into request_params) and pass it to _http_client.call() to make the API behave as documented.
| timeout='medium', | |
| timeout=request_params.get('timeout', 'medium'), |
| def __init__( | ||
| self, | ||
| *, | ||
| token: str | None = None, | ||
| timeout: timedelta = DEFAULT_TIMEOUT, | ||
| timeout_short: timedelta = DEFAULT_TIMEOUT_SHORT, | ||
| timeout_medium: timedelta = DEFAULT_TIMEOUT_MEDIUM, | ||
| timeout_long: timedelta = DEFAULT_TIMEOUT_LONG, | ||
| timeout_max: timedelta = DEFAULT_TIMEOUT_MAX, | ||
| max_retries: int = DEFAULT_MAX_RETRIES, |
There was a problem hiding this comment.
HttpClientBase.__init__ removed the previously supported timeout parameter. Since HttpClient/HttpClientAsync are public ABCs intended for subclassing, this is a breaking change for custom clients that call super().__init__(timeout=...). Consider keeping timeout as a deprecated keyword-only alias (e.g., mapping it to timeout_long and/or setting all tiers) to preserve backward compatibility until the next major release.
| min_delay_between_retries: timedelta = DEFAULT_MIN_DELAY_BETWEEN_RETRIES, | ||
| timeout: timedelta = DEFAULT_TIMEOUT, | ||
| timeout_short: timedelta = DEFAULT_TIMEOUT_SHORT, | ||
| timeout_medium: timedelta = DEFAULT_TIMEOUT_MEDIUM, | ||
| timeout_long: timedelta = DEFAULT_TIMEOUT_LONG, | ||
| timeout_max: timedelta = DEFAULT_TIMEOUT_MAX, |
There was a problem hiding this comment.
ApifyClient.__init__ no longer accepts the previous timeout parameter, which is a breaking change for existing users on a 2.x release line. Consider re-introducing timeout as a deprecated keyword-only argument (e.g., mapping to timeout_long / setting all tiers) and emitting a DeprecationWarning to allow a gradual migration.
| min_delay_between_retries: timedelta = DEFAULT_MIN_DELAY_BETWEEN_RETRIES, | ||
| timeout: timedelta = DEFAULT_TIMEOUT, | ||
| timeout_short: timedelta = DEFAULT_TIMEOUT_SHORT, | ||
| timeout_medium: timedelta = DEFAULT_TIMEOUT_MEDIUM, | ||
| timeout_long: timedelta = DEFAULT_TIMEOUT_LONG, | ||
| timeout_max: timedelta = DEFAULT_TIMEOUT_MAX, |
There was a problem hiding this comment.
ApifyClientAsync.__init__ no longer accepts the previous timeout parameter, which is a breaking change for existing users on a 2.x release line. Consider re-introducing timeout as a deprecated keyword-only argument (e.g., mapping to timeout_long / setting all tiers) and emitting a DeprecationWarning to allow a gradual migration.
Pijukatel
left a comment
There was a problem hiding this comment.
I think it is good if we want to expose the timeouts on the API client method level as well. But are there any usecases for it?
Users already have the option to change to some extent the timeout behavior by giving their own HTTP client - like client that inherits from ImpitHttpClient and just modifies the timeout-based behavior.
| headers=headers, | ||
| content=content, | ||
| timeout=self._calculate_timeout(attempt, timeout), | ||
| timeout=self._compute_timeout(timeout, attempt), |
There was a problem hiding this comment.
Out of curiosity: What will be the behavior of impit when timeout is None ? Is it some impit default or no timeout at all?
| restart_on_error: bool | None = None, | ||
| memory_mbytes: int | None = None, | ||
| timeout: timedelta | None = None, | ||
| run_timeout: timedelta | None = None, |
There was a problem hiding this comment.
I agree with this, but it makes this change breaking or not?
|
@janbuchar could you please take a look, as such refactoring will probably make its way to JS version as well. It would be for the best to comment on any changes now. |
Summary
short(5s by default),medium(30s by default),long(360s by default),max(360s cap by default).Timeouttype alias:timedelta | Literal['no_timeout', 'short', 'medium', 'long']timeout_max)._internal_models.pyto_types.pyand consolidate type aliases (Timeout,JsonSerializable) into it.FAST_OPERATION_TIMEOUT,STANDARD_OPERATION_TIMEOUT).Timeout tiers
shortmediumlongno_timeoutmaxLet's discuss the best defaults later in a dedicated PR.
What this solves
Test plan